Relax fileExtension requirement in attachmentQueue #269
Conversation
There was a problem hiding this comment.
Thanks for the work done here. I recognise some of the current limitations the Dart Attachments helper has. This is an improvement.
I recently worked on the Kotlin and Swift Attachment helpers. I also noticed this inconvenience during development and opted for a different approach which might align better with your thought of
I'm wondering if it shouldn't be pushed further to be able to specify a Map<String, String?> for <filename, extension?>
The Kotlin and Swift implementations allow the watched query to return more than just the attachment ID. They allow returning an array of "partial attachments" which can include a filename and fileExtension. These values are resolved if not provided.
I personally am in favour of updating the watched query result to contain a partial Attachment record type which can contain more information.
|
Hi @stevensJourney, thanks for the feedback! It's interesting to see that the question raised in other technos as well and that a common ground has been found to tackle this issue. Of course, I think consistency is key as Powersync users expect it when using the library from one stack to another. I'll have a deeper look in your changes and try to adapt my PR asap 🤓 |
…tion (Is a directory)
n0mad-d3v
force-pushed
the
feature/file-extension
branch
from
405056c to
21d6427
Compare
There was a problem hiding this comment.
First of all, apologies for the delay in reviewing this. We are also working on a rewrite of the attachments helper package (#311) based on lessons learned writing similar APIs for our Kotlin and Swift SDKs.
That doesn't mean these changes aren't valuable, and we can still merge and release this since it's an immediate improvement. But I want to point out that it would likely get replaced in the near future.
| Directory parentDirectories = File(fileUri).parent; | ||
| await parentDirectories.create(recursive: true); |
There was a problem hiding this comment.
Is this change necessary? Looking at how the method is called in init, it looks like it's really supposed to create a directory at fileUri. This only creates the parent directory.
I think creating nested directories automatically is the correct behavior, but then you could call await makeDir(dirname(targetUri)). The same thing could also be reasonable for saveFile.
| @@ -59,7 +64,7 @@ abstract class AbstractAttachmentQueue { | |||
|
|
|||
| /// Create watcher to get list of ID's from a table to be used for syncing in the attachment queue. | |||
| /// Set the file extension if you are using a different file type | |||
| StreamSubscription<void> watchIds({String fileExtension = 'jpg'}); | |||
| StreamSubscription<void> watchIds({String? fileExtension}); | |||
There was a problem hiding this comment.
Considering that this is already a breaking change, I wonder if we should just remove the parameter entirely and transfer the responsibility to include file extensions in the id instead of supporting both modes.
That would also involve removing the fileExtension parameter from processIds.
| @@ -165,11 +165,12 @@ class SyncingService { | |||
| } | |||
|
|
|||
| /// Process ID's to be included in the attachment queue. | |||
| Future<void> processIds(List<String> ids, String fileExtension) async { | |||
| Future<void> processIds(List<String> ids, String? fileExtension) async { | |||
There was a problem hiding this comment.
To follow up on Steven's suggestion, we could remove fileExtension here and replace List<String> ids with something like List<WatchedAttachmentItem>, where WatchedAttachmentItem is a simple class with a String id and a String? extension field.
Then here, we could take the extension of each item or fall back to the default extension set on the AbstractAttachmentQueue.
Hello powersync team 👋
This PR fixes #268 and updates the watchIds method in the Attachments helper to make the fileExtension parameter optional. This enables support for storing files with different extensions in the same attachmentQueue, which is useful in cases where:
Motivation: Currently, the API assumes a single extension per queue, which limits use cases that involve multiple file types. Making fileExtension optional opens up broader usage while maintaining backward compatibility.
This change was inspired by this Discord thread.
WDYT? Looking forward to feedback!
PS: There is also a change about ensuring directory creation. In my usecase, I'm using Teams' ids as subdirectory so it's fully dynamic and my filename is like the following:
{teamId}/{attachmentType}/{attachmentId}.{attachmentExtension}.